-
Notifications
You must be signed in to change notification settings - Fork 289
profiles: use single Profile.sample_type #649
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Implements open-telemetry#633 (comment). Signed-off-by: Florian Lehner <florian.lehner@elastic.co> Co-authored-by: Felix Geisendörfer <felix.geisendoerfer@datadoghq.com>
Should default sample type go to the scope message? |
Co-authored-by: Christos Kalkanis <christos.kalkanis@elastic.co>
That would be less flexible as it'd force us to have separate instrumentation scopes ( Do you have a use-case in mind where associating different sample types with separate instrumentation scopes would be a benefit? (I don't have a strong opinion on this, just that if we don't have a good reason to force a separate instrumentation scope per sample type, I'd go for more flexibility). |
I mostly want to make sure there is some way to capture the default sample type. The current PR removes the field IIUC. |
The main purpose of the proposal is to align values with timestamps as laid out in #633 (comment). In general google/pprof it is possible to have multiple |
@aalexand (I misread your previous comment and thought you wanted to lift |
As one example, Go heap profiles have four sample types today:
The question is how such a profile with four sample types and one of them being the preferred default as chosen by the profile producer should be encoded in OpenTelemetry profiles. Before this change it would be the array of sample types plus the default sample type. What would it be after this change and how would the default be captured? |
The current proposal doesn't have an explicit default sample type (and we probably should steer away from introducing implicit conventions) so assuming four different sample types in a single OTel messsage, samples will be grouped according to sample type to separate profiles. One possible encoding would have four different profiles, each with a different sample type (of course one could further split the data according to other criteria). Some options to accommodate the notion of a default sample type (e.g. for a consumer visualization tool) include:
1 is more restricted than 2 as it could be seen as purely advisory metadata that a visualization tool can use. Adding |
I think using nanoseconds is the better default unit for CPU profiles because the weight of a count can be different depending on the sample rate. Using the same unit for off_cpu and cpu is more consistent. The comment above was a little confusing, since it referred only to a cpu profile, but was then followed by an off_cpu profile sample as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM in general. Sorry for the delay. PTAL at my comments. (I still need to find time to reply to some of the comments above, will do soon)
profiles: improve sample_type comment
…-proto/pull/649/files#r2083519081 Signed-off-by: Florian Lehner <florian.lehner@elastic.co>
Signed-off-by: Florian Lehner <florian.lehner@elastic.co>
I have added
I didn't mark |
…e_type Signed-off-by: Florian Lehner <florian.lehner@elastic.co>
|
||
// The preferred type and unit of Samples in at least one Profile. | ||
// See Profile.sample_type for possible values. | ||
ValueType default_sample_type = 4; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it need to be a dedicated field? Could it be a scope attribute?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could use an attribute (it was the other option here) but then we'd need to ensure key uniqueness and possibly introduce a new semantic convention.
Given that all fields are optional in proto3, and that this specific attribute seems only relevant to profiling, it's simpler I think to have this field here? We could also update the field documentation to mention that it's optional.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Profiling SIG discussed this topic and agreed to prefer a dedicated field over some other mechanism.
Implements #633 (comment).
FYI: @open-telemetry/profiling-approvers